Skip to content

Conversation

@dreab8
Copy link
Member

@dreab8 dreab8 commented Jul 24, 2025

Fix #1906

}

@Override
default Id generate(

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method ReactiveIdentifierGenerator.generate(..) could be confused with overloaded method
generate
, since dispatch depends on static types.
}

@Override
default Object generate(SharedSessionContractImplementor session, Object object){

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method ReactiveIdentifierGenerator.generate(..) could be confused with overloaded method
generate
, since dispatch depends on static types.
Copy link
Member

@DavideD DavideD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I gave it a quick look, but I will check better tomorrow

public void testPersist(VertxTestContext context) {
Person person = new Person();
test(
context, openSession()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use getMutinySessionFactory.withTransaction?
I think it's more similar to a typical use case.

Maybe we could also test without transactions, but we should at least add a comment to explain what's going on

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to also read the value from the database to make sure that they match? Maybe it's overkill

context, openSession()
.thenCompose( session -> session.persist( person ) )
.thenAccept( v -> {
assertThat( person.getId() ).isNotNull();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that this test is wrong because the id is a primitive type so it's never going to be null

@DavideD
Copy link
Member

DavideD commented Aug 7, 2025

I will send a PR rebased to the latest changes

@DavideD DavideD closed this Aug 7, 2025
@DavideD DavideD mentioned this pull request Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support @IdGeneratorType

2 participants